Skip to content

fix: prevent term-select from overriding current term on initial load#198

Open
daynayoon wants to merge 2 commits intomainfrom
selected-term-behavior
Open

fix: prevent term-select from overriding current term on initial load#198
daynayoon wants to merge 2 commits intomainfrom
selected-term-behavior

Conversation

@daynayoon
Copy link
Copy Markdown
Contributor

  • Problem
    • On initial load, term-select.tsx had a fallback effect that ran when selectedTermId was null, racing against the current term fetch in class-list-view.tsx and overwriting it with terms[0].
  • Fix
    • Added selectedTermId && guard so the fallback only runs when a previously-set term no longer exists (e.g. deleted), not on initial load.

@daynayoon daynayoon self-assigned this Apr 4, 2026
@daynayoon daynayoon linked an issue Apr 4, 2026 that may be closed by this pull request
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 4, 2026

Greptile Summary

This PR fixes a race condition on initial page load where term-select.tsx's fallback useEffect was firing while selectedTermId was still null, overwriting the in-flight current-term fetch in class-list-view.tsx with terms[0]. The one-line selectedTermId && guard correctly scopes the fallback to the case where a previously-selected term has been deleted.

  • Fix: Added selectedTermId && guard in the fallback useEffect so it only fires when a term that was previously selected no longer exists — not on the initial render when selectedTermId is null.
  • Comment updated: The inline comment now correctly documents the narrower intent of the effect.
  • Minor edge case: The guard removes a pre-existing implicit fallback for when queryTerm === \"current\" but no term is configured as current. If that scenario can occur, the class-list-view.tsx sync effect should handle it explicitly rather than relying on term-select.tsx as a side-effect fallback.

Confidence Score: 4/5

Safe to merge; the fix resolves the described race condition with a minor edge-case consideration.

Single-line, targeted change with clear rationale backed by the class-list-view.tsx implementation. A small regression risk exists if no 'current' term is ever configured, but this is an unlikely edge case in normal usage.

src/components/classes/list/components/term-select.tsx — verify behaviour when no current term is set in the database

Important Files Changed

Filename Overview
src/components/classes/list/components/term-select.tsx Adds selectedTermId && guard to prevent the fallback effect from overriding initial term selection on load

Reviews (1): Last reviewed commit: "fix: prevent term-select from overriding..." | Re-trigger Greptile

Comment on lines 38 to 42
useEffect(() => {
const selectedTerm = terms?.find((t) => t.id === selectedTermId);
if (!selectedTerm && terms?.[0] && !isRefetching)
if (selectedTermId && !selectedTerm && terms?.[0] && !isRefetching)
setSelectedTermId(terms[0].id);
}, [terms, selectedTermId, setSelectedTermId]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Potential regression when no current term exists

The guard is correct for the described race condition, but it removes a safety net for a related edge case. When queryTerm === "current" but no term is marked as current (i.e., currentTerm is undefined), the effect in class-list-view.tsx (lines 112–116) never sets selectedTermId, leaving it as null indefinitely. Previously this useEffect would have rescued the UI by falling back to terms[0]; with selectedTermId && added, the guard prevents that fallback and the component renders a loading skeleton forever.

If it's always guaranteed that a current term exists when there are any terms, this is fine. Otherwise, consider adding a fallback in class-list-view.tsx for when queryTerm === "current" but currentTerm is absent:

// In class-list-view.tsx, inside the sync effect (around line 112)
if (queryTerm === "current") {
  if (!selectedTermId && currentTerm) {
    setSelectedTermId(currentTerm.id);
  } else if (!selectedTermId && !isLoadingCurrentTerm && terms?.[0]) {
    // No current term configured; fall back to the first term
    setSelectedTermId(terms[0].id);
  }
  return;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Strange selected term behavior

1 participant